@itswisdomagain and I chatted about this and I think it's a great idea. It opens the door to other Decred wallets like https://github.com/planetdecred/godcr, maybe something mobile long-term, maybe a built-in, etc. It takes an approach to a Wallet interface similar to what was done in #1089 , and it generally seems like a clean change that doesn't affect the existing RPC wallets.

The last commit 4a920ae adopts #1230's asset.Driver changes with some modifications to allow alternative wallet implementations aside from the builtin wallets being implemented by #1225 and #1230. One modification with semantic importance is CreateWallet -> PrepareWallet, to accommodate cases where some work is required before the ExchangeWallet can be used but not wallet creation either.

Here's an idea of how the new PrepareWallet method can be used to prepare a dcr ExchangeWallet that provides wallet-specific functionalities using a *decred.org/dcrwallet/v2/wallet.Wallet instance that is connected to a syncer (network backend) without having to register a separate driver:

func PrepareSpvWallet(w *wallet.Wallet, desc string) {
	spvWalletInfo := dcr.WalletInfo
	spvWalletInfo.DefaultConfigPath = ""
	spvWalletInfo.ConfigOpts = configOpts // defined elsewhere

	asset.PrepareWallet(dcr.BipID, &dcr.PrepareWalletParams{
		Wallet: &SpvWallet{ // SpvWallet implements the dcr.Wallet interface using the *wallet.Wallet instance
			Wallet: w,
			desc:   desc,
		},
		WalletInfo: spvWalletInfo,
	})
}

The above works as long as it is called before asset.LoadWallet(dcr.BipID, ...).

Thoughts @chappjc @buck54321 @martonp?

@itswisdomagain I don't understand your change. The point of the CreateWallet method, which you are renaming to PrepareWallet, is to provide a seed for the wallet. I don't see how this would happen here.

Agree with @martonp. Have a look at how the various (*Driver).Create implementations work in PR #1230. The first thing is that this really is creation, so I think "prepare" is actually less clear. But my other question is, what does asset.CreateWalletParams not permit that requires using an interface{} instead? EDIT: I think I'm being too narrow minded regarding where the actual implementation of the wallet lives. See my following comment. Leaving the text of my original comment here however.

It also seems to me that the caller of this asset.PrepareWallet (current called asset.Create) would not want to know about types like SpvWallet at all, only certain types identified by some string or other enumerator. e.g. if params.Type != WalletTypeSPV { in that PR's impl of btc's (*Driver).Create

Say dex were trying to use a new hypothetical DCR backend that is say a dcrlibwallet-based built-in... what is insufficient about asset.CreateWalletParams for Create? A hypothetical godcr external would not use Create at all because presumably the user would have to have created their wallet externally first, just like with dcrwallet external, and just go to Open.

Note that we've recently decided that Exists will only apply to internal/seeded wallets that were made by a previous call to Create.

OK I think I'm looking at this too rigidly, thinking that any possible dcr.Wallet interface implementation would need to be defined in the client/asset/dcr package.

But I think you're looking at an external package that implements dcr.Wallet and passes that to the asset.Driver methods as an interface{}, and when the dcr package gets it, it would ensure it satisfies dcr.Wallet, and if so, use that when doing Open (presumably if the WalletConfig.Type agrees with something from the provided Wallet)?

So in this scenario, would it not be better to leave Create alone and add a UseWallet(interface{}) for this different approach?

I'm now aware that @buck54321 has thoughts about doing something similar, except by registering a constructor for a particular wallet type, so I'll leave this to you guys to sort out.

would it not be better to leave Create alone and add a UseWallet(interface{}) for this different approach?

Works for me. I went with PrepareWallet to be generic, rather than add a new interface method that really only applies to one asset (for now at least). The way I think of it, CreateWallet is being introduced now because there is now a need to perform some preparatory work of sorts before using the regular Setup/Open/Load. Renaming to PrepareWallet lets individual asset implementations do their thing, whatever that is, to get the wallet ready for loading. Assets that support seeded/inbuilt wallets can use that method to receive needed wallet creation params and create the wallet, while assets like dcr can use that method to receive an object that satisfies the dcr.Wallet interface and use it for wallet functionality. One method defined in the interface, different possible implementations depending on the asset and wallet types supported. Could even modify PrepareWallet(interface{}) to PrepareWallet(type string, params interface{}).

That said, UseWallet(interface{}) will definitely work for this purpose, if that's the general preference.

EDIT:
After a little more thought, asset.UseWallet(assetID, interface{}) could be confusing, since we have a asset.OpenWallet/LoadWallet function, with both functions offering different interpretations of the term Wallet. Also, the Wallet in UseWallet is a really only an internal construct used and defined by a few assets and not an asset-generic item. I'll still prefer a PrepareWallet(interface{}) method (or some other name?), even if a separate CreateWallet method is added to create inbuilt wallets.

The idea is, some assets (dcr for now) might require some preparatory works of sort before they can be loaded. So while for most assets, it'd be sufficient to call asset.Setup/OpenWallet/LoadWallet, some assets might require a preceding call to asset.PrepareWallet to ensure that the wallet is ready to be loaded and started/connected.

The point of the CreateWallet method, which you are renaming to PrepareWallet, is to provide a seed for the wallet. I don't see how this would happen here.

For seeded/inbuilt wallets, Core just calls asset.PrepareWallet instead of asset.CreateWallet, and passes in the same *asset.CreateWalletParams. The assets that support wallet creation can then convert the params provided to *CreateWalletParams and do their thing, while other assets like dcr can convert the params to dcr.Wallet and do its thing.

How about a package-level method

Just processing this, this could/should work, although I do not understand what purpose the WalletConstructor type would serve.

I'm thinking of the following for the dcr asset:

var wallet &rpcWallet // default too for wallet-specific functionality
var walletLoaded uint32 // atomic var, set to 1 after `NewWallet` is called to prevent modifying `wallet`.

func UseCustomWallet(wallet Wallet, def *asset.WalletDefinition) // sets the wallet var and appends to AvailableWallets

Although, I wonder the implication of adding wallet types dynamically, e.g. but can probably deal with that after the fact.

Also, would this mean each asset that want to support multiple ways of providing wallet functionality will have to do likewise? The only other alternative I see is adding new methods to the asset.Driver interface to enable wallets support new wallet types, such as is being done for the seeded/inbuilt wallets and as I attempted in my previous commit. I think I still prefer a driver.PrepareWallet method for assets to use in making magic happen.

The assets that support wallet creation can then convert the params provided to *CreateWalletParams and do their thing, while other assets like dcr can convert the params to dcr.Wallet and do its thing.

This is why I think these are two separate functions. One needs CreateWalletParams and one needs a Wallet/WalletConstructor (whichever is yet to be decided). Both are followed by Open/Load (whichever is yet to be decided). But just taking an interface{} is confusing.

The answer to constructor vs. wallet interface provided to RegisterCustomWallet/UseCustomWallet is: whichever most effectively separates the caller from the concerns of the underlying custom wallet implementation. With a constructor as the arg, the caller does not need to make the actual wallet, only inform the asset package what constructor is to be used to make a wallet for a particular WalletDefinition, then they can use Open/Load once they've registered that constructor. It keeps the setup workflow more standard.

With a constructor as the arg, the caller does not need to make the actual wallet, only inform the asset package what constructor is to be used to make a wallet for a particular WalletDefinition

Missed that the constructor is to make a Wallet not ExchangeWallet. Sounds like it would work smoothly. Will give it a go.

just taking an interface{} is confusing

Yeah, I agree. Whether that's why there should be separate methods on the Driver interface is where I'm less sure. Not to push it, but my reasoning is while both require different parameters and do different things, they share a similarity - getting the wallet ready to be loaded/used with asset.Open/Load. As I said in my last comment, other wallet setup processes could come up for any asset, would that require a new interface method each time? Perhaps, this is where asset-specific package level functions would come in handy. In any case, will cross that bridge when we get there. I do like that instead of having a new UseWallet/PrepareWallet/SetupWallet method that really only applies to dcr (for now at least), a dcr package-level function would be better.

Now using a package-level function as suggested by @buck54321. Not using WalletDefinition yet, that comes with some wide-reaching changes, would rather have #1230 go in first, then I rebase this to use WalletDefinition.

Converted to draft for now, want to do a little cleanup before this will be ready for review again.